-
Notifications
You must be signed in to change notification settings - Fork 154
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Direct Get by start time #1226
Conversation
Signed-off-by: Maurice van Veen <[email protected]>
* server such as timeout or interruption | ||
* @throws JetStreamApiException the request had an error related to the data | ||
*/ | ||
MessageInfo getFirstMessage(String streamName, ZonedDateTime startTime) throws IOException, JetStreamApiException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there ever a case when we will want first message by subject and start time? It's probably fine to have this since it seems like a common behavior, but I think it's time to expose something like
MessageInfo getMessage(String streamName, MessageGetRequest messageGetRequest)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added, in combination with the new builder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some discussion offline removed the builder again, as the only combinations that needed to be added were:
getFirstMessage(streamName, startTime)
getFirstMessage(streamName, startTime, subject)
And then we don't need extra validation in the builder for valid combinations.
} | ||
|
||
private MessageGetRequest(long sequence, String lastBySubject, String nextBySubject) { | ||
private MessageGetRequest(long sequence, String lastBySubject, String nextBySubject, ZonedDateTime startTime) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably should add a builder to this class, especially if we are going to add a generic get message api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the builder 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -1305,6 +1306,7 @@ private void validateGetMessage(JetStreamManagement jsm, TestingStreamContainer | |||
|
|||
assertStatus(10003, assertThrows(JetStreamApiException.class, () -> jsm.getMessage(tsc.stream, -1))); | |||
assertStatus(10003, assertThrows(JetStreamApiException.class, () -> jsm.getMessage(tsc.stream, 0))); | |||
assertStatus(10003, assertThrows(JetStreamApiException.class, () -> jsm.getFirstMessage(tsc.stream, DEFAULT_TIME))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, is it correct that this fails because the start time is before the very first message in the stream?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't fail because of the start time being before the first message.
Instead it fails because a DEFAULT_TIME
doesn't get serialized into the final JSON, resulting in an empty JSON object being sent which is checked here should be an error.
Signed-off-by: Maurice van Veen <[email protected]>
Signed-off-by: Maurice van Veen <[email protected]>
Signed-off-by: Maurice van Veen <[email protected]>
Drafting for now, needs a little more polish to capture the valid combinations in |
Signed-off-by: Maurice van Veen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: Maurice van Veen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.